Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Session cost add message #1007

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

maaikez
Copy link
Contributor

@maaikez maaikez commented Mar 6, 2025

Describe your changes

Do not use the display message interface to send session cost messages, but use a specific session cost message type.

Issue ticket number and link

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have made corresponding changes to the documentation
  • [n/a] If OCPP 2.0.1 or OCPP2.1: I have updated the OCPP 2.x status document
  • I read the contribution documentation and made sure that my changes meet its requirements

Comment on lines 3176 to 3168
if (!messages.empty()) {
session_cost_message.message = messages;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And you can drop this

Suggested change
if (!messages.empty()) {
session_cost_message.message = messages;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?
then the messages are not included?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should now be able to drop

#include <ocpp/v2/functional_blocks/display_message.hpp>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not build in that case: it is missing message_content_to_display_message_content then

maaikez added 3 commits March 7, 2025 17:00
…cing information.

Signed-off-by: Maaike Zijderveld, iolar <git.mail@iolar.nl>
Signed-off-by: Maaike Zijderveld, iolar <git.mail@iolar.nl>
Signed-off-by: Maaike Zijderveld, iolar <git.mail@iolar.nl>
@maaikez maaikez force-pushed the feature/session-cost-add-message branch from 343ea0a to 07368e3 Compare March 7, 2025 16:00
Copy link
Contributor

@Pietfried Pietfried left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, please see just a couple of remarks inline


## OCPP 2.0.1

OCPP 2.0.1 uses different mechanisms to send pricing information. The messages are converted to internally used structs as descripbed above. For California Pricing Requirements to work, DisplayMessage and TariffAndCost must be implemented as well.
OCPP 2.0.1 uses different mechanisms to send pricing information. The messages are converted to internally used structs as descripbed above. For California Pricing Requirements to work, TariffAndCost must be implemented as well.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
OCPP 2.0.1 uses different mechanisms to send pricing information. The messages are converted to internally used structs as descripbed above. For California Pricing Requirements to work, TariffAndCost must be implemented as well.
OCPP 2.0.1 uses different mechanisms to send pricing information. The messages are converted to internally used structs as described above. For California Pricing Requirements to work, TariffAndCost must be implemented as well.


For the tariff information (the personal messages), the `set_display_message_callback` is used. The same callback is also used for the SetDisplayMessageRequest in OCPP. The latter does require an id, the former will not have an id. So when `GetDisplayMessageRequest` is called from the CSMS, the Tariff display messages (that do not have an id) should not be returned. They should also be removed as soon as the transaction has ended.
For the tariff information (the personal messages), the `session_cost_message_callback` is used.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
For the tariff information (the personal messages), the `session_cost_message_callback` is used.
For the tariff information (the personal messages), the `session_cost_message_callback` is used.

@@ -161,6 +161,9 @@ struct Callbacks {
std::optional<std::string> currency_code)>>
set_running_cost_callback;

/// \brief Callback function is called when tariff message is set.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// \brief Callback function is called when tariff message is set.
/// \brief Callback function is called when a tariff message from the CSMS message has been received.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would help to give a little bit more context here and to describe whats expected from the callback.

@@ -573,6 +573,12 @@ class ChargePoint {
const std::function<DataTransferResponse(const RunningCost& session_cost, const uint32_t number_of_decimals)>&
session_cost_callback);

/// \brief Registers a callback function for the session cost text message (California Pricing Requirements).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would help to give a little bit more context here and to describe whats expected from the callback.

Signed-off-by: Piet Gömpel <pietgoempel@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants